Skip to content

Replace obsolete ENV VAR VALUE with ENV VAR=VALUE #2254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Replace obsolete ENV VAR VALUE with ENV VAR=VALUE #2254

wants to merge 1 commit into from
+54 −54

Conversation

orgads
Copy link
Contributor

@orgads orgads commented Jul 8, 2025

Description

Fixes these warnings:

- LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format

Testing Details

Build passed.

Types of changes

  • Documentation
  • Version change (Update, remove or add more Node.js versions)
  • Variant change (Update, remove or add more variants, or versions of variants)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (none of the above)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING.md document.
  • All new and existing tests passed.

@orgads orgads mentioned this pull request Jul 8, 2025
Draft
12 tasks
@PeterDaveHello PeterDaveHello requested a review from Copilot July 10, 2025 18:05
Copilot

This comment was marked as outdated.

Sign in to view

PeterDaveHello
PeterDaveHello requested changes Jul 10, 2025
View reviewed changes
Copy link
Member

@PeterDaveHello PeterDaveHello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to fix update.sh & functions.sh

@orgads
Copy link
Contributor Author

orgads commented Jul 10, 2025

Fixed. This PR now depends on:

The failing test is because https://news.ycombinator.com is down. I reported it, hope they'll fix it soon.

@orgads
Copy link
Contributor Author

orgads commented Jul 10, 2025

It's up again. All should pass now.

@orgads orgads requested review from PeterDaveHello and Copilot July 10, 2025 19:45
Copilot

This comment was marked as outdated.

Sign in to view

@orgads orgads requested a review from Copilot July 10, 2025 19:54
Copilot

This comment was marked as outdated.

Sign in to view

@PeterDaveHello
Copy link
Member

Why should this one depend on #2258?

Fixed. This PR now depends on:

The failing test is because https://news.ycombinator.com is down. I reported it, hope they'll fix it soon.

@orgads
Copy link
Contributor Author

orgads commented Jul 10, 2025

Why should this one depend on #2258?

Because they touch the same line in update.sh.

@PeterDaveHello
Copy link
Member

That's something should be prevented.

I don't want to mix different topic in the same pull request.

Why should this one depend on #2258?

Because they touch the same line in update.sh.

@orgads
Copy link
Contributor Author

orgads commented Jul 10, 2025

I agree. So which one of them do you want to accept first? You can either merge the other one and then I'll rebase this one, or I can swap them.

PeterDaveHello
PeterDaveHello requested changes Jul 10, 2025
View reviewed changes
Copy link
Member

@PeterDaveHello PeterDaveHello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove unrelated commit changes that are not relevant to the topic.

@PeterDaveHello
Copy link
Member

I agree. So which one of them do you want to accept first? You can either merge the other one and then I'll rebase this one, or I can swap them.

Maybe this one as it was sent earlier.

@orgads orgads closed this Jul 10, 2025
@orgads orgads requested a review from PeterDaveHello July 10, 2025 20:12
@orgads
Copy link
Contributor Author

orgads commented Jul 10, 2025

Done

@orgads orgads reopened this Jul 10, 2025
@PeterDaveHello PeterDaveHello requested a review from Copilot July 11, 2025 17:12
Copilot

This comment was marked as outdated.

Sign in to view

PeterDaveHello
PeterDaveHello reviewed Jul 11, 2025
View reviewed changes
PeterDaveHello
PeterDaveHello reviewed Jul 11, 2025
View reviewed changes
@orgads
Replace obsolete ENV VAR VALUE with ENV VAR=VALUE
b8d8680 
Fixes these warnings:
- LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy
  "ENV key value" format
@orgads orgads requested review from PeterDaveHello and Copilot July 12, 2025 18:31
Copilot
Copilot AI reviewed Jul 12, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the Dockerfile ENV directives from the legacy ENV KEY VALUE format to the preferred ENV KEY=VALUE syntax and adjusts supporting scripts to match.

  • update.sh, stackbrew.js, genMatrix.js, and functions.sh are modified to parse and emit ENV KEY=VALUE entries.
  • Documentation and templates (docs/BestPractices.md, README.md, Dockerfile-*.template) are updated to reflect the new syntax.
  • All versioned Dockerfiles in the 20/, 22/, and 24/ directories are converted to use ENV KEY=VALUE.

Reviewed Changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
update.sh Adjust sed rules to match and emit ENV KEY=VALUE.
stackbrew.js Update regex to extract NODE_VERSION with =.
genMatrix.js Update regex to extract full node version with =.
functions.sh Use grep/cut -d= for ENV NODE_VERSION=.
docs/BestPractices.md Change examples to ENV YARN_VERSION=....
README.md Update example to ENV NPM_CONFIG_LOGLEVEL=info.
Dockerfile-slim.template Switch ENV NODE_VERSION and YARN_VERSION to =.
Dockerfile-debian.template Switch ENV NODE_VERSION and YARN_VERSION to =.
Dockerfile-alpine.template Switch ENV NODE_VERSION and YARN_VERSION to =.
24/bullseye/Dockerfile Convert ENV syntax for node and yarn versions.
24/bullseye-slim/Dockerfile Convert ENV syntax for node and yarn versions.
24/bookworm/Dockerfile Convert ENV syntax for node and yarn versions.
24/bookworm-slim/Dockerfile Convert ENV syntax for node and yarn versions.
24/alpine3.22/Dockerfile Convert ENV syntax for node and yarn versions.
24/alpine3.21/Dockerfile Convert ENV syntax for node and yarn versions.
22/bullseye/Dockerfile Convert ENV syntax for node and yarn versions.
22/bullseye-slim/Dockerfile Convert ENV syntax for node and yarn versions.
22/bookworm/Dockerfile Convert ENV syntax for node and yarn versions.
22/bookworm-slim/Dockerfile Convert ENV syntax for node and yarn versions.
22/alpine3.22/Dockerfile Convert ENV syntax for node and yarn versions.
22/alpine3.21/Dockerfile Convert ENV syntax for node and yarn versions.
20/bullseye/Dockerfile Convert ENV syntax for node and yarn versions.
20/bullseye-slim/Dockerfile Convert ENV syntax for node and yarn versions.
20/bookworm/Dockerfile Convert ENV syntax for node and yarn versions.
20/bookworm-slim/Dockerfile Convert ENV syntax for node and yarn versions.
20/alpine3.22/Dockerfile Convert ENV syntax for node and yarn versions.
20/alpine3.21/Dockerfile Convert ENV syntax for node and yarn versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@PeterDaveHello PeterDaveHello Awaiting requested review from PeterDaveHello

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@orgads @PeterDaveHello